-
Notifications
You must be signed in to change notification settings - Fork 21
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add support to vulnerability aliases #220
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! Thank you @macedogm
Just one nit, can you add a simple validation to vexStatementOptions.Validate()
? I think for now checking we are not duplicating identifiers in vexStatementOptions.Aliases
and vexStatementOptions.Vulnerability
should be enough. (Oh and also add a test case to options_test.go )
vexctl/internal/cmd/options.go
Lines 72 to 77 in 8d81805
// Validate checks that the statement options are coherent | |
func (so *vexStatementOptions) Validate() error { | |
if so.Status != string(vex.StatusAffected) && | |
(so.ActionStatement != vex.NoActionStatementMsg && so.ActionStatement != "") { | |
return errors.New("action statement can only be set when status = \"affected\" ") | |
} |
@puerco thanks for the quick review and comments. Yes, I'll add the improvements that you requested, just a note that I'll have to travel for a few days, so there might be some delay before I update the PR. |
@macedogm if you need this change we can handle the validation in a follow up. If you need this one quickly, just rebase the PR, we merge it and I can add the validation logic :) |
@puerco I can wait a few days, no rush on that. It's more up to you if you prefer to merge this one quickly and do the rest on a follow up PR. |
7b1da57
to
8770741
Compare
Ah the linter is not happy, otherwise LGTM |
Signed-off-by: Guilherme Macedo <[email protected]>
8770741
to
8a9459b
Compare
@puerco hopefully I addressed your comments. PTAL and let me know if the style is as expected. Additionally, should we consider adding a validation in |
The vexStatementOptions are embedded in the addOptions, so your change should work in both :) Lines 36 to 40 in 9bbf7cd
Thank you! |
This is an initial implementation that adds support to vulnerability aliases as documented in the spec.
Note that support is only added when creating a new Vex document or adding a new statement. It doesn't support filtering or merging based on aliases.
Closes #219